Skip to content

Failing tests: placeholder gate rejects valid reordered human translations#25802

Merged
jkmassel merged 7 commits into
jkmassel/catalog-strings-translationfrom
catalog-strings-positional-gate-regression
Jul 18, 2026
Merged

Failing tests: placeholder gate rejects valid reordered human translations#25802
jkmassel merged 7 commits into
jkmassel/catalog-strings-translationfrom
catalog-strings-positional-gate-regression

Conversation

@oguzkocer

Copy link
Copy Markdown
Contributor

Heads-up on a regression I hit reviewing #25713. This PR is failing tests only — no fix — so you can merge it and fix in place (or fold the fix into #25713 directly).

TranslationValidator compares a source's positional and sequential specifier views independently, so it only allows reordering when the source is already positional. A non-positional source (%@ … %@) whose translation reorders via positional specifiers (%2$@ … %1$@) is rejected — even though that's the standard, Apple-documented iOS reordering mechanism and is correct at runtime (String(format:) honors positional specifiers regardless of the source shape).

The human-translation gate this PR adds (CatalogStrings.trusted_human, PluralStrings.human_forms_for) inherits this and downgrades valid, currently-shipping human translations to machine/English. Real data: 23 key-as-source strings across 34 locales (513 shipping cells) do exactly this, e.g. ar "%@ of %@ used on your site""%1$@ من %2$@ على موقعك".

Failing tests

  • translation_validator_test.rbtest_positionalizing_a_non_positional_source_to_reorder_is_allowed — the root cause at the validator.
  • catalog_strings_helper_test.rbtest_reordered_human_translation_of_a_non_positional_source_is_kept_as_translated — the user-facing downgrade in the fold.

Only these two are red; everything else passes.

Suggested fix

In TranslationValidator (shared by AI, plurals, and regular human strings): treat a non-positional source as implicitly numbered 1..N by appearance order, and accept a fully-positional candidate whose index→(length:type) map matches that implied map. The existing tests already keep genuine breakage rejected — a sequential flip (%@: %d%d : %@) and type/count changes — so the fix can't simply drop the positional/sequential distinction.

@oguzkocer
oguzkocer requested a review from jkmassel July 17, 2026 23:13
@oguzkocer
oguzkocer requested a review from a team as a code owner July 17, 2026 23:13
@oguzkocer oguzkocer added the Tooling Build, Release, and Validation Tools label Jul 17, 2026
@oguzkocer oguzkocer added this to the 27.1 milestone Jul 17, 2026
…nslations

TranslationValidator compares a source's positional and sequential specifier
views independently, so it only permits reordering when the source is ALREADY
positional. A non-positional source (`%@ … %@`) whose translation reorders via
positional specifiers (`%2$@ … %1$@`) is rejected — even though that is the
standard, Apple-documented iOS reordering mechanism and is correct at runtime
(`String(format:)` honors positional specifiers regardless of the source shape).

The human-translation gate added for the String Catalog fold
(`CatalogStrings.trusted_human`, `PluralStrings.human_forms_for`) inherits this,
so it rejects and downgrades valid, currently-shipping human translations to
machine/English. This is not hypothetical: the committed translated `.strings`
contain 23 key-as-source strings across 34 locales (513 cells) that positionalize
a bare-`%@` English source this way, e.g. ar "%@ of %@ used on your site" =>
"%1$@ من %2$@ على موقعك".

Two failing tests pin the correct behavior:
- translation_validator_test.rb — the root cause at the validator.
- catalog_strings_helper_test.rb — the user-facing downgrade in the fold.

The fix belongs in TranslationValidator (shared by AI, plurals, and regular
human strings): treat a non-positional source as implicitly numbered 1..N by
appearance order and accept a fully-positional candidate whose index→type map
matches. Existing tests already keep genuine breakage (sequential flip, type/
count change) rejected, so a correct fix cannot just drop the distinction.
@oguzkocer
oguzkocer force-pushed the catalog-strings-positional-gate-regression branch from 777cc6e to 92bf21f Compare July 17, 2026 23:19
jkmassel added 6 commits July 18, 2026 14:12
…ource

TranslationValidator compared a source's positional and sequential specifier
views independently, so it only permitted reordering when the source was ALREADY
positional. A non-positional source (`%@ … %@`) whose translation reorders via
positional specifiers (`%2$@ … %1$@`) was rejected — even though that is the
standard, Apple-documented iOS mechanism and is correct at runtime, because
`String(format:)` honors positional specifiers regardless of the source's shape.

Normalize a purely-sequential signature into the implicit 1..N positional map its
bare specifiers already carry (`%@ %d` => `{1 => object, 2 => int}`) before
comparing, applied to both source and candidate. Once both sides are index→type
maps, `%@ - %@` and `%2$@ - %1$@` compare equal and the reorder is accepted.

Genuine breakage stays rejected: a sequential flip `%@: %d` => `%d : %@` yields
`{1 => object, 2 => int}` vs `{1 => int, 2 => object}` — a disagreement on which
index owns which type — and type/length/count changes are unaffected. A signature
that already uses positional specifiers, or one that mixes bare and positional
specifiers (malformed), is left unchanged, so mixed strings still require an exact
structural match.

Fixes the two failing tests added in the previous commit and propagates to all
three gate consumers (AI, plurals, regular human strings) via the shared
validator. All 7 fastlane/lanes suites pass (113 tests); rubocop clean.
…e cases

Three currently-red tests documenting real defects found auditing the
placeholder gate and the build-free coverage compare:

- A candidate that binds the same positional index to two different types
  (%1$@ %1$d) is accepted: signature() collapses it last-wins, and this PR's
  implied_positional widened the reach to bare-specifier sources (%d), which
  regressed from reject to accept.
- Dynamic field width/precision (%*d, %.*f) consume an extra int vararg the
  signature never counts, so %d and %*d reduce to the same signature.
- coverage_gap conflates keys that differ only in argument type (%d days vs
  %@ days), masking a genuinely dropped key behind a same-prose sibling.
…ting type

signature() stored positional specifiers in a Hash keyed by index with plain
assignment, so a second reference to the same index (`%1$@` then `%1$d`)
silently overwrote the first, collapsing an object/int conflict to one token.
A candidate that references argument 1 as both an object and an int passes an
int to a `%@` conversion at runtime — a wrong-vararg read. This PR's
implied_positional widened the reach: a bare-specifier source like `%d` now
normalizes to {1 => int} and matched the collapsed candidate, regressing that
case from reject to accept.

Record a conflicting reuse as a distinct token so it can never equal a
well-formed source's single type, while still allowing a consistent reuse
(`%1$d … %1$d`, which legally reads one argument twice).
A `*` in a specifier's field width or precision (`%*d`, `%.*f`) consumes its
own int argument before the value, so `%d` and `%*d` are not interchangeable.
The FORMAT_SPECIFIER regex matched the `*` but signature() emitted no token
for it, so the two reduced to the same signature and a translation could add
or drop a dynamic-width/precision argument undetected — reading one vararg
past what the call site supplies. Emit an int argument for each `*`, in
appearance order before the value.
canonical() replaced every format specifier with one sentinel regardless of
type, so keys differing only in argument type (`%d days` vs `%@ days`)
collapsed to the same coverage cell — a genuinely-dropped key was masked by a
same-prose sibling of a different type, defeating the same-basename overwrite
regression the coverage gate exists to catch. Strip only the positional index
(the sole difference between the genstrings source-form `%li` and the
xcstringstool-normalized `%1$li` for the same source literal), so `%li` and
`%1$li` still compare equal while `%d` and `%@` stay distinct.
… class

Danger's RuboCop pass flagged two offenses from the previous commits:

- Metrics/AbcSize: signature() exceeded the ABC limit once it gained the
  positional-conflict and dynamic-width handling. Extract record_specifier
  and add_positional so signature() is a thin loop again; behavior unchanged.
- Style/OneClassPerFile: the coverage_gap test added a second top-level class
  to catalog_helper_test.rb. Fold it into the file's single test class,
  renamed CatalogHelperTest since it now covers both reword detection and the
  coverage compare.
@jkmassel
jkmassel merged commit 8e9af7e into jkmassel/catalog-strings-translation Jul 18, 2026
26 checks passed
@jkmassel
jkmassel deleted the catalog-strings-positional-gate-regression branch July 18, 2026 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Tooling Build, Release, and Validation Tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants